Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rewrite intrinsic-unreachable, sepcomp-cci-copies, sepcomp-inlining and sepcomp-separate run-make tests to rmake.rs #126427

Merged
merged 3 commits into from
Jul 8, 2024

Conversation

Oneirical
Copy link
Contributor

Part of #121876 and the associated Google Summer of Code project.

@rustbot
Copy link
Collaborator

rustbot commented Jun 13, 2024

r? @jieyouxu

rustbot has assigned @jieyouxu.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added A-run-make Area: port run-make Makefiles to rmake.rs A-testsuite Area: The testsuite used to check the correctness of rustc S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) labels Jun 13, 2024
@rustbot
Copy link
Collaborator

rustbot commented Jun 13, 2024

The run-make-support library was changed

cc @jieyouxu

This PR modifies tests/run-make/. If this PR is trying to port a Makefile
run-make test to use rmake.rs, please update the
run-make port tracking issue
so we can track our progress. You can either modify the tracking issue
directly, or you can comment on the tracking issue and link this PR.

cc @jieyouxu

These commits modify the Cargo.lock file. Unintentional changes to Cargo.lock can be introduced when switching branches and rebasing PRs.

If this was unintentional then you should revert the changes before this PR is merged.
Otherwise, you can ignore this comment.

@Oneirical Oneirical force-pushed the oktobertest branch 2 times, most recently from 954fc4f to bf298cb Compare June 13, 2024 19:22
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

Comment on lines 258 to 267
paths.filter_map(|entry| entry.ok()).filter(|path| path.is_file()).for_each(|path| {
let file = fs_wrapper::open_file(path);
let reader = io::BufReader::new(file);
reader
.lines()
.filter_map(|line| line.ok())
.filter(|line| re.is_match(line))
.for_each(|_| match_count += 1);
});
match_count
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a feeling this can be streamlined using the .count() method to remove the need to manually increment, but I don't know what the most idiomatic style would be

Copy link
Contributor Author

@Oneirical Oneirical Jun 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

    paths.filter_map(|entry| entry.ok())
        .filter(|entry| entry.as_path().is_file())
        .filter_map(|path| fs::File::open(&path).ok())
        .map(|file| io::BufReader::new(file))
        .flat_map(|reader| reader.lines().filter_map(|entry| entry.ok()))
        .filter(|line| re.is_match(line))
        .count()

Good suggestion, after some tinkering I got this to work. It feels a little bulky, though.

@Kobzol Kobzol changed the title Rewrite intrinsic-unreachable, sepcomp-separate, sepcomp-inlining and sepcomp-separate run-make tests to rmake.rs Rewrite intrinsic-unreachable, sepcomp-cci-copies, sepcomp-inlining and sepcomp-separate run-make tests to rmake.rs Jun 13, 2024
@rust-log-analyzer

This comment has been minimized.

tests/run-make/intrinsic-unreachable/rmake.rs Outdated Show resolved Hide resolved
tests/run-make/sepcomp-separate/Makefile Show resolved Hide resolved
Comment on lines 248 to 263
/// Inside a glob pattern of files (paths), read their contents and count the
/// number of regex matches with a given expression (re).
#[track_caller]
pub fn count_regex_matches_in_file_glob(re: &str, paths: &str) -> usize {
let re = regex::Regex::new(re).expect(format!("Regex expression {re} is not valid.").as_str());
let paths = glob::glob(paths).expect(format!("Glob expression {paths} is not valid.").as_str());
use io::BufRead;
paths
.filter_map(|entry| entry.ok())
.filter(|entry| entry.as_path().is_file())
.filter_map(|path| fs::File::open(&path).ok())
.map(|file| io::BufReader::new(file))
.flat_map(|reader| reader.lines().filter_map(|entry| entry.ok()))
.filter(|line| re.is_match(line))
.count()
}
Copy link
Member

@jieyouxu jieyouxu Jun 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Discussion: a globbing API makes me very uneasy, I would strongly prefer if this was a walkdir-style API (and specialized helpers built on such an API, see src/tools/tidy's usage of walkdir-style APIs) instead, because it's not immediately clear if this is recursive, deals with "hidden" files, handles non-UTF8, among other filesystem / glob-related possible failstates and gotchas.

Copy link
Contributor Author

@Oneirical Oneirical Jun 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have tried my hand at a reworked version of this that does not use glob - see the latest commit. Tell me what you think!

fn main() {
rustc().input("cci_lib.rs").run();
rustc().input("foo.rs").emit("llvm-ir").codegen_units(6).arg("-Zinline-in-all-cgus").run();
assert_eq!(count_regex_matches_in_file_glob(r#"define\ .*cci_fn"#, "foo.*.ll"), 2);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Discussion: cf. the support library comment, I feel like this helper API makes it almost too opaque for the test reader of what is being tested.

@jieyouxu
Copy link
Member

@rustbot author

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 16, 2024
@Oneirical Oneirical force-pushed the oktobertest branch 2 times, most recently from cbf8157 to dc989e7 Compare June 17, 2024 15:54
@rust-log-analyzer

This comment has been minimized.

@jieyouxu
Copy link
Member

Note to myself: revisit this PR @rustbot review

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 17, 2024
@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 19, 2024
@bors
Copy link
Contributor

bors commented Jun 19, 2024

☔ The latest upstream changes (presumably #126607) made this pull request unmergeable. Please resolve the merge conflicts.

@jieyouxu
Copy link
Member

I think we should just avoid using walkdir because there's some funny windows-targets related business going down (tried debugging, no luck). We can just build it upon fs::read_dir() et al.

@rustbot author

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 29, 2024
@Oneirical
Copy link
Contributor Author

Notes for myself:

  • Use shallow_find_files instead of walkdir.
  • In intrinsic-unreachable, use fs_wrapper::read_to_string instead of open_file.

@Oneirical Oneirical force-pushed the oktobertest branch 2 times, most recently from 4bbe6b9 to 64a7343 Compare July 5, 2024 15:06
@Oneirical
Copy link
Contributor Author

@rustbot review

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 5, 2024
Copy link
Member

@jieyouxu jieyouxu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests look good, r=me after hoisting the helper into the support lib.

Comment on lines 14 to 24
fn count_regex_matches_in_files_with_extension(re: &regex::Regex, ext: &str) -> usize {
let fetched_files = shallow_find_files(cwd(), |path| has_extension(path, ext));

let mut count = 0;
for file in fetched_files {
let content = fs_wrapper::read_to_string(file);
count += content.lines().filter(|line| re.is_match(&line)).count();
}

count
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: can you hoist this into the support library and add some docs for what it does? I know I may have said to keep it localized in the test files previously, sorry!

@jieyouxu
Copy link
Member

jieyouxu commented Jul 6, 2024

@rustbot author

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 6, 2024
@Oneirical Oneirical force-pushed the oktobertest branch 2 times, most recently from 8e89c65 to 1c69496 Compare July 8, 2024 14:13
@Oneirical
Copy link
Contributor Author

@bors r=@jieyouxu

@bors
Copy link
Contributor

bors commented Jul 8, 2024

📌 Commit f04d0c6 has been approved by jieyouxu

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 8, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 8, 2024
…llaumeGomez

Rollup of 4 pull requests

Successful merges:

 - rust-lang#126427 (Rewrite `intrinsic-unreachable`, `sepcomp-cci-copies`, `sepcomp-inlining` and `sepcomp-separate` `run-make` tests to rmake.rs)
 - rust-lang#127237 (Improve code of `run-make/llvm-ident`)
 - rust-lang#127325 (Migrate `target-cpu-native`,  `target-specs` and `target-without-atomic-cas` `run-make` tests to rmake)
 - rust-lang#127482 (Infer async closure signature from (old-style) two-part `Fn` + `Future` bounds)

Failed merges:

 - rust-lang#127357 (Remove `StructuredDiag`)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 9804cf9 into rust-lang:master Jul 8, 2024
6 checks passed
@rustbot rustbot added this to the 1.81.0 milestone Jul 8, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Jul 8, 2024
Rollup merge of rust-lang#126427 - Oneirical:oktobertest, r=jieyouxu

Rewrite `intrinsic-unreachable`, `sepcomp-cci-copies`, `sepcomp-inlining` and `sepcomp-separate` `run-make` tests to rmake.rs

Part of rust-lang#121876 and the associated [Google Summer of Code project](https://blog.rust-lang.org/2024/05/01/gsoc-2024-selected-projects.html).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-run-make Area: port run-make Makefiles to rmake.rs A-testsuite Area: The testsuite used to check the correctness of rustc S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

7 participants